Add support for periodic columns in LogUp-GKR #307

Al-Kindi-0 · 2024-09-03T19:53:41Z

No description provided.

irakliyk

Thank you! Looks good! I left some comments inline - the main one is about assuming that periodic oracles being always at the end of the list of oracles. But not sure if that's a big issue.

sumcheck/Cargo.toml

winterfell/src/tests/logup_gkr_simple.rs

winterfell/src/tests/mod.rs

air/src/air/logup_gkr/mod.rs

sumcheck/src/verifier/mod.rs

irakliyk · 2024-09-03T22:42:57Z

sumcheck/src/verifier/mod.rs

+    let mut query = proof.0.openings_claim.openings.clone();
+    query.extend_from_slice(&periodic_columns_evaluations);


Here we are assuming that periodic column values are always at the end of the query, right? But when we define oracles, I don't think this is enforced. Could this cause issues?

Indeed we are, and this will break things if that is not the case. Should we add checks and panic throughout to enforce this?

If we always expect it to be at the end of the query buffer, can't we simply not pass it to build_query(), and append it ourselves?

IIUC, there is no other correct way to do it, so there's no point in asking (and hoping for) the user to do it.

Agreed. I think we should try to derive oracles for periodic columns so that the user does not need to make sure that oracles are ordered correctly.

Also, maybe we define query something like this:

pub struct Query<E: FieldElement> { pub committed_values: Vec<E>, pub periodic_value: Vec<E>, }

This way, there is no ambiguity about where the periodic values should go (though, it does require 2 vectors instead of 1).

If we always expect it to be at the end of the query buffer, can't we simply not pass it to build_query(), and append it ourselves?

I am not sure how build_query() fits into the code snippet above. build_query() is specifically for building from the main trace segment.

Agreed. I think we should try to derive oracles for periodic columns so that the user does not need to make sure that oracles are ordered correctly.

I am not sure I understand fully but the way I see solving the issue raised is by separating the the oracles into committed and periodic.
My assumption was that, if we are going to test the consistency between get_oracles() and build_query() anyway then we might as well impose that periodic columns are at the end. The test will check for this any and imposing that periodic columns are towards the end is implicit in requiring consistency between get_oracles() and build_query().

I am not sure how build_query() fits into the code snippet above. build_query() is specifically for building from the main trace segment.

We are currently making an assumption about the query argument to evaluate_query() - namely that the periodic columns need to be appended at the end of the buffer. Hence, this assumption needs to be upheld by build_query() as well.

My point was: why give the user the possibly to screw it up and place the periodic columns somewhere other than at the end of the query buffer? We could instead change build_query() to be

fn build_query<E>(&self, frame: &EvaluationFrame<E>, query: &mut [E]) where E: FieldElement<BaseField = Self::BaseField>, { ... }

and append periodic values manually after each call to build_query(), for example here:

evaluator.build_query(&main_frame, &mut query); query.extend(periodic_values_row);

i.e. we are enforcing that assumption ourselves, and not leaving the user any possibility for mistake.

IIUC, @irakliyk built on this and suggested we make the structure explicit by representing the periodic values separately (instead of appended at the end up the buffer). So the previous example would look like

evaluator.build_query(&main_frame, &mut query); let query_full = Query::new(query, periodic_values_row);

Does that make sense? Maybe I'm missing something too.

Yep, it does make sense. One thing that is different with respect to the new proposal is with respect to new allocations. In the current way, we just write to a buffer but with the current proposal I am not sure if we are incurring some kind of overhead when creating Query.

I think Query::new() should be fine here - i.e., it would not allocate anything as it would leave on the stack.

sumcheck/src/prover/high_degree.rs

plafer · 2024-09-04T18:24:04Z

air/src/air/logup_gkr/mod.rs

+                table.push(values)
+            }
+        }
+        PeriodicTable { table }


I would rewrite this as

let table: Vec<Vec<F>> = self .get_oracles() .iter() .filter_map(|oracle| { if let LogUpGkrOracle::PeriodicValue(values) = oracle { Some(values.into_iter().copied().map(F::from).collect()) } else { None } }) .collect(); PeriodicTable{ table }

E is not used, and it is redundant, right? It seems like we could always make F::BaseField the basefield, and F the extension field (or F::BaseField and F both be the base field).

Thanks for the suggestion, switched to it
Indeed, the generic is redundant now, it is a leftover from a previous iteration. Removed now

air/src/air/logup_gkr/mod.rs

prover/src/logup_gkr/prover.rs

plafer · 2024-09-04T18:37:04Z

sumcheck/src/prover/high_degree.rs

@@ -160,6 +160,7 @@ pub fn sum_check_prove_higher_degree<
    r_sum_check: E,
    log_up_randomness: Vec<E>,
    mut mls: Vec<MultiLinearPoly<E>>,
+    periodic_table: &mut PeriodicTable<E>,
    coin: &mut impl RandomCoin<Hasher = H, BaseField = E::BaseField>,


Suggested change

coin: &mut impl RandomCoin<Hasher = H, BaseField = E::BaseField>,

mut periodic_table: PeriodicTable<E>,

We can pass this to sumcheck_round() as &mut periodic_table

winterfell/src/tests/logup_gkr_periodic.rs

prover/src/logup_gkr/mod.rs

plafer · 2024-09-04T19:13:20Z

sumcheck/src/verifier/mod.rs

+    let mut query = proof.0.openings_claim.openings.clone();
+    query.extend_from_slice(&periodic_columns_evaluations);


If we always expect it to be at the end of the query buffer, can't we simply not pass it to build_query(), and append it ourselves?

IIUC, there is no other correct way to do it, so there's no point in asking (and hoping for) the user to do it.

plafer

Looks good! Left a few nits.

air/src/air/logup_gkr/mod.rs

irakliyk

Overall, looks good! Thank you! I do wonder if we should change things slight based on the discussion in #307 (comment). Basically, it seems like adding periodic values to the list of other oracles imposes some implicit assumptions. So, maybe we should just treat them separately (and we kind of do in a few places already).

One way to do this could be as follows:

First, we remove PeriodicValue variant from the LogUpGkrOracle enum.

Then, we add a dedicated method to the LogUpGkrEvaluator to get periodic columns. Something like:

pub trait LogUpGkrEvaluator {
    fn get_periodic_column_values(&self) -> Vec<Vec<Self::BaseField>>;
}

Then, we restrict LogUpGkrEvaluator::build_query() to work only with committed columns. So, it would look something like this:

pub trait LogUpGkrEvaluator {
    fn build_query<E>(&self, frame: &EvaluationFrame<E>, query: &mut [E])
    where
        E: FieldElement<BaseField = Self::BaseField>;
}

Finally, we pass periodic values to the LogUpGkrEvaluator::evaluate_query() explicitly like so:

pub trait LogUpGkrEvaluator {
    fn evaluate_query<F, E>(
        &self,
        query: &[F],
        periodic_values: &[F],
        logup_randomness: &[E],
        numerators: &mut [E],
        denominators: &mut [E],
    ) where
        F: FieldElement<BaseField = Self::BaseField>,
        E: FieldElement<BaseField = Self::BaseField> + ExtensionOf<F>;
}

This way, we don't make any implicit assumptions about where periodic values go in the query - they are always treated separately.

Lastly, the implementation of LogUpGkrEvaluator ::build_periodic_values() would be simplified as well.

irakliyk · 2024-09-09T06:38:52Z

One other question: with the suggestions I made above, it looks like handling of periodic values in LogUpGkrEvaluator is almost the same as in the main Air trait. Should we just keep the periodic columns defined in the Air trait? (i.e., basically the user defines all periodic column in the Air trait and they can be used both for regular constraints and for LogUp-GKR.

I guess the answer to this depends on how much overhead this will introduce as we may end up with some periodic values passed to LogUpGkrEvaluator which it doesn't really need.

Al-Kindi-0 · 2024-09-09T13:10:08Z

Overall, looks good! Thank you! I do wonder if we should change things slight based on the discussion in #307 (comment). Basically, it seems like adding periodic values to the list of other oracles imposes some implicit assumptions. So, maybe we should just treat them separately (and we kind of do in a few places already).

Implemented your suggestion, although the simplifications in some parts come at the expense of less uniformity in the high degree sum-check prover. I will create an issue to optimize the way periodic column (multi-linears) are handled there as I am sure the succinctness of these column can be leveraged in order to optimize the implementation with respect to them.

Al-Kindi-0 · 2024-09-09T13:11:39Z

One other question: with the suggestions I made above, it looks like handling of periodic values in LogUpGkrEvaluator is almost the same as in the main Air trait. Should we just keep the periodic columns defined in the Air trait? (i.e., basically the user defines all periodic column in the Air trait and they can be used both for regular constraints and for LogUp-GKR.

I guess the answer to this depends on how much overhead this will introduce as we may end up with some periodic values passed to LogUpGkrEvaluator which it doesn't really need.

That's correct, but maybe when building the periodic table for LogUp-GKR we can return only the values figuring in LogUp-GKR.

irakliyk

Looks good! Thank you! I left one small comment inline (but we can also merge w/o it and fix it later).

irakliyk · 2024-09-09T21:50:50Z

sumcheck/src/prover/high_degree.rs

@@ -280,21 +298,28 @@ fn sumcheck_round<E: FieldElement>(
    evaluator: &impl LogUpGkrEvaluator<BaseField = <E as FieldElement>::BaseField>,
    eq_ml: &MultiLinearPoly<E>,
    mls: &[MultiLinearPoly<E>],
+    periodic_table: &mut PeriodicTable<E>,


Does this need to be &mut PeriodicTable<E>? Doesn't seem like we are mutating the table in this function.

Good point! Fixed

Al-Kindi-0 added 20 commits August 6, 2024 14:49

feat: math utilities needed for sum-check protocol

8ac25c7

feat: add sum-check prover and verifier

5e06378

tests: add sanity tests for utils

16389d6

doc: document sumcheck_round

380aa1a

feat: use SmallVec

7a1a99e

docs: improve documentation of sum-check

1901066

feat: add remaining functions for sum-check verifier

8a57216

chore: move prover into sub-mod

ff9e6fa

chore: remove utils mod

7e24f8f

chore: remove utils mod

23044e8

chore: move logup evaluator trait to separate file

ad0497d

feat: add GKR backend for LogUp-GKR

a0272ea

chore: remove old way of handling Lagrange kernel

7b8caff

wip: add s-column constraints

e2b8c12

chore: correct header

b813916

wip

492f247

wip: add support for periodic columns in gkr backend

0d664e0

Merge branch 'logup-gkr' into al-gkr-periodic

8617308

chore: fix post merge issues

ed781d8

chore: fix issues

98c0e71

facebook-github-bot added the cla signed label Sep 3, 2024

Al-Kindi-0 changed the title ~~Add support for periodic columns in LogUp-GKR (version 2)~~ Add support for periodic columns in LogUp-GKR Sep 3, 2024

doc: add comment about periodic values table

807aba1

irakliyk reviewed Sep 3, 2024

View reviewed changes

Al-Kindi-0 added 2 commits September 4, 2024 11:34

chore: address feedback

4e6d3ab

chore: fix concurrent portion

c93ec35

plafer reviewed Sep 4, 2024

View reviewed changes

irakliyk reviewed Sep 5, 2024

View reviewed changes

air/src/air/logup_gkr/mod.rs Outdated Show resolved Hide resolved

chore: address feedback

873345a

irakliyk reviewed Sep 9, 2024

View reviewed changes

chore: address feedback

d94794a

irakliyk approved these changes Sep 9, 2024

View reviewed changes

Al-Kindi-0 added 2 commits September 10, 2024 12:21

chore: remove unnecessary mut

b484e04

chore: remove unnecessary mut

dec6589

irakliyk merged commit b5f64cc into facebook:logup-gkr Sep 10, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for periodic columns in LogUp-GKR #307

Add support for periodic columns in LogUp-GKR #307

Al-Kindi-0 commented Sep 3, 2024

irakliyk left a comment

irakliyk Sep 3, 2024

Al-Kindi-0 Sep 4, 2024

plafer Sep 4, 2024

irakliyk Sep 5, 2024

Al-Kindi-0 Sep 5, 2024

Al-Kindi-0 Sep 5, 2024

plafer Sep 5, 2024

Al-Kindi-0 Sep 6, 2024

irakliyk Sep 9, 2024

plafer Sep 4, 2024

Al-Kindi-0 Sep 5, 2024

plafer Sep 4, 2024

Al-Kindi-0 Sep 5, 2024

plafer Sep 4, 2024

plafer left a comment

irakliyk left a comment

irakliyk commented Sep 9, 2024

Al-Kindi-0 commented Sep 9, 2024

Al-Kindi-0 commented Sep 9, 2024

irakliyk left a comment

irakliyk Sep 9, 2024

Al-Kindi-0 Sep 10, 2024

		let mut query = proof.0.openings_claim.openings.clone();
		query.extend_from_slice(&periodic_columns_evaluations);

	coin: &mut impl RandomCoin<Hasher = H, BaseField = E::BaseField>,
	mut periodic_table: PeriodicTable<E>,

Add support for periodic columns in LogUp-GKR #307

Add support for periodic columns in LogUp-GKR #307

Conversation

Al-Kindi-0 commented Sep 3, 2024

irakliyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

irakliyk left a comment

Choose a reason for hiding this comment

irakliyk commented Sep 9, 2024

Al-Kindi-0 commented Sep 9, 2024

Al-Kindi-0 commented Sep 9, 2024

irakliyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment